feat: Make MCP server actions more robust#820
Conversation
🦋 Changeset detectedLatest commit: 2c0cd6c The changes in this PR will be included in the next version bump. This PR includes changesets to release 21 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds auto-fix support and fix application for draft project definitions, plugin management and entity search actions, refactors draft validation/save pipeline to produce fix metadata and warnings, extends schema path model to include array/record path elements, and updates tests and action registry accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI/Client
participant Actions as ServiceAction(apply-fix)
participant Draft as DraftSession (storage)
participant Parser as Schema Parser/Validator
participant Serializer as Serializer
participant FS as File System
CLI->>Actions: request apply-fix(project, fixId)
Actions->>Draft: getOrCreateDraftSession(project)
Actions->>Parser: build container from draft (parse)
Parser->>Actions: collect issues with fix metadata
Actions->>Actions: find issue by fixId
Actions->>Parser: apply fix to parsed definition (immer produce)
Actions->>Serializer: serialize fixed definition to name-based format
Actions->>Parser: fixAndValidateDraftDefinition(fixedSerializedDef)
Parser->>Actions: return errors/warnings and new container
Actions->>Draft: validateAndSaveDraft(..., session, projectDir)
Actions->>FS: write draft-definition.json
Actions->>CLI: return message + remaining issues
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (6)
packages/project-builder-server/src/actions/definition/search-entities.action.ts (1)
52-63: Consider adding result limiting for large entity sets.The search returns all matching entities without pagination or a result limit. For projects with many entities, this could return unexpectedly large result sets. Consider adding an optional
limitparameter to cap results.💡 Optional enhancement for result limiting
const searchEntitiesInputSchema = z.object({ project: z.string().describe('The name or ID of the project.'), query: z .string() .describe('Case-insensitive substring to match against entity names.'), entityTypeName: z .string() .optional() .describe('Restrict search to a specific entity type.'), + limit: z + .number() + .int() + .positive() + .optional() + .default(100) + .describe('Maximum number of results to return.'), });Then in the handler:
const results = container.entities .filter((entity) => { if (input.entityTypeName && entity.type.name !== input.entityTypeName) { return false; } return entity.name.toLowerCase().includes(queryLower); }) + .slice(0, input.limit) .map((entity) => ({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-server/src/actions/definition/search-entities.action.ts` around lines 52 - 63, The current search in the results assignment returns all matches; add an optional numeric limit parameter (e.g., input.limit) and apply it to cap returned entities to a sane default (e.g., 100) when provided, validating it is a positive integer; locate the results creation that uses container.entities.filter(...).map(...) and either slice the filtered array before mapping or slice the mapped array (use .slice(0, limit)) so only up to the requested number of items are returned, and update the handler signature/validation to accept and sanitize input.limit.packages/project-builder-server/src/actions/definition/definition-test-fixtures.test-helper.ts (2)
79-79: Replace empty object pattern with underscore placeholder.Biome flags the empty object pattern
{}. Use_as a conventional placeholder for unused parameters.♻️ Proposed fix
- testData: async ({}, use) => { + testData: async (_, use) => { await use(buildTestData()); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-server/src/actions/definition/definition-test-fixtures.test-helper.ts` at line 79, The parameter list for the async arrow function assigned to testData uses an empty object pattern "{}", which Biome flags; replace that with the conventional unused-parameter placeholder "_" so change the function signature in testData (the async ({}, use) => handler) to use "_" for the first argument (async (_, use) =>) to indicate the parameter is intentionally unused.
91-91: Replace empty object pattern with underscore placeholder.Same issue as above — use
_for the unused parameter.♻️ Proposed fix
- projectDir: async ({}, use) => { + projectDir: async (_, use) => { await use('/test-project'); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-server/src/actions/definition/definition-test-fixtures.test-helper.ts` at line 91, The async arrow for projectDir is using an empty object pattern for an unused parameter (projectDir: async ({}, use) => { }); replace the empty destructuring with a single underscore placeholder (e.g., change the first parameter to _ or _args) to signal the parameter is intentionally unused; update the parameter in the projectDir async function definition only.packages/project-builder-server/src/actions/definition/disable-plugin.action.ts (1)
59-62: Consider using the more common Immerproducesignature.The curried form works but is less readable. The standard two-argument form is clearer:
♻️ Optional: Use standard produce signature
- // Apply disablePlugin via produce - const newDefinition = produce((draft: ProjectDefinition) => { - PluginUtils.disablePlugin(draft, input.pluginKey, parserContext); - })(container.definition); + // Apply disablePlugin via produce + const newDefinition = produce(container.definition, (draft) => { + PluginUtils.disablePlugin(draft, input.pluginKey, parserContext); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-server/src/actions/definition/disable-plugin.action.ts` around lines 59 - 62, Replace the curried Immer call with the standard two-argument produce signature to improve readability: call produce with the base state first and the drafting function second (use produce(container.definition, (draft: ProjectDefinition) => { PluginUtils.disablePlugin(draft, input.pluginKey, parserContext); })), keeping the same references to PluginUtils.disablePlugin, ProjectDefinition, input.pluginKey, parserContext and container.definition so behavior is unchanged.packages/project-builder-server/src/actions/definition/disable-plugin.action.unit.test.ts (1)
16-25: Consider adding a happy-path test for successful plugin disabling.Currently only the error case is tested. A test verifying successful disabling would improve coverage and catch regressions in the core flow.
Would you like me to help draft a happy-path test case?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-server/src/actions/definition/disable-plugin.action.unit.test.ts` around lines 16 - 25, Add a happy-path unit test that sets up a project with a plugin already enabled, invokes disablePluginAction via invokeServiceActionForTest with { project: 'test-project', pluginKey: '<enabled-key>' }, and asserts the call resolves successfully; then fetch the project state (e.g., via the test context's project service/getProject method) and assert the pluginKey is no longer present in the project's enabled plugins list. Use the existing test suite for disablePluginAction and mirror the error-test structure to keep setup/teardown consistent.packages/project-builder-server/src/actions/definition/stage-delete-entity.action.int.test.ts (1)
75-81: Use name-based lookup instead of array index for robustness.The first test correctly uses
definition.models.some((m) => m.name === 'BlogPost')to verify by name. This test should follow the same pattern instead of assumingmodels[0]is BlogPost—the index could change if fixtures are modified or additional models are added.♻️ Suggested fix
const definition = JSON.parse(defContents) as { - models: { model: { fields: { name: string }[] } }[]; + models: { name: string; model: { fields: { name: string }[] } }[]; }; - const blogPost = definition.models[0]; + const blogPost = definition.models.find((m) => m.name === 'BlogPost'); + if (!blogPost) throw new Error('BlogPost model not found in definition'); const fieldNames = blogPost.model.fields.map((f) => f.name);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-server/src/actions/definition/stage-delete-entity.action.int.test.ts` around lines 75 - 81, The test currently assumes the BlogPost model is at definition.models[0]; update it to find the model by name instead: locate the block using the variables definition, models, blogPost and fieldNames and replace the index-based access (definition.models[0]) with a name-based lookup such as definition.models.find(m => m.name === 'BlogPost'), assert the result is non-null (throw or expect) and then use that found blogPost.model.fields.map(f => f.name) for the same contains/notContains assertions to make the test robust to fixture ordering changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/project-builder-server/src/actions/definition/definition-test-fixtures.test-helper.ts`:
- Line 79: The parameter list for the async arrow function assigned to testData
uses an empty object pattern "{}", which Biome flags; replace that with the
conventional unused-parameter placeholder "_" so change the function signature
in testData (the async ({}, use) => handler) to use "_" for the first argument
(async (_, use) =>) to indicate the parameter is intentionally unused.
- Line 91: The async arrow for projectDir is using an empty object pattern for
an unused parameter (projectDir: async ({}, use) => { }); replace the empty
destructuring with a single underscore placeholder (e.g., change the first
parameter to _ or _args) to signal the parameter is intentionally unused; update
the parameter in the projectDir async function definition only.
In
`@packages/project-builder-server/src/actions/definition/disable-plugin.action.ts`:
- Around line 59-62: Replace the curried Immer call with the standard
two-argument produce signature to improve readability: call produce with the
base state first and the drafting function second (use
produce(container.definition, (draft: ProjectDefinition) => {
PluginUtils.disablePlugin(draft, input.pluginKey, parserContext); })), keeping
the same references to PluginUtils.disablePlugin, ProjectDefinition,
input.pluginKey, parserContext and container.definition so behavior is
unchanged.
In
`@packages/project-builder-server/src/actions/definition/disable-plugin.action.unit.test.ts`:
- Around line 16-25: Add a happy-path unit test that sets up a project with a
plugin already enabled, invokes disablePluginAction via
invokeServiceActionForTest with { project: 'test-project', pluginKey:
'<enabled-key>' }, and asserts the call resolves successfully; then fetch the
project state (e.g., via the test context's project service/getProject method)
and assert the pluginKey is no longer present in the project's enabled plugins
list. Use the existing test suite for disablePluginAction and mirror the
error-test structure to keep setup/teardown consistent.
In
`@packages/project-builder-server/src/actions/definition/search-entities.action.ts`:
- Around line 52-63: The current search in the results assignment returns all
matches; add an optional numeric limit parameter (e.g., input.limit) and apply
it to cap returned entities to a sane default (e.g., 100) when provided,
validating it is a positive integer; locate the results creation that uses
container.entities.filter(...).map(...) and either slice the filtered array
before mapping or slice the mapped array (use .slice(0, limit)) so only up to
the requested number of items are returned, and update the handler
signature/validation to accept and sanitize input.limit.
In
`@packages/project-builder-server/src/actions/definition/stage-delete-entity.action.int.test.ts`:
- Around line 75-81: The test currently assumes the BlogPost model is at
definition.models[0]; update it to find the model by name instead: locate the
block using the variables definition, models, blogPost and fieldNames and
replace the index-based access (definition.models[0]) with a name-based lookup
such as definition.models.find(m => m.name === 'BlogPost'), assert the result is
non-null (throw or expect) and then use that found blogPost.model.fields.map(f
=> f.name) for the same contains/notContains assertions to make the test robust
to fixture ordering changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a03dd0fc-6c65-42d6-95e2-74391a6a9443
📒 Files selected for processing (38)
.changeset/mcp-server-actions.md.changeset/mcp-server-improvements.mdpackages/project-builder-lib/src/parser/walk-schema-structure.tspackages/project-builder-lib/src/parser/walk-schema-structure.unit.test.tspackages/project-builder-lib/src/tools/assign-entity-ids.unit.test.tspackages/project-builder-lib/src/tools/entity-service/entity-navigation.tspackages/project-builder-lib/src/tools/entity-service/entity-type-map.tspackages/project-builder-server/src/actions/__tests__/action-test-utils.tspackages/project-builder-server/src/actions/definition/apply-fix.action.tspackages/project-builder-server/src/actions/definition/commit-draft.action.tspackages/project-builder-server/src/actions/definition/configure-plugin.action.tspackages/project-builder-server/src/actions/definition/configure-plugin.action.unit.test.tspackages/project-builder-server/src/actions/definition/definition-actions.int.test.tspackages/project-builder-server/src/actions/definition/definition-test-fixtures.test-helper.tspackages/project-builder-server/src/actions/definition/disable-plugin.action.tspackages/project-builder-server/src/actions/definition/disable-plugin.action.unit.test.tspackages/project-builder-server/src/actions/definition/draft-lifecycle.int.test.tspackages/project-builder-server/src/actions/definition/entity-type-blacklist.tspackages/project-builder-server/src/actions/definition/get-entity-schema.action.unit.test.tspackages/project-builder-server/src/actions/definition/get-entity.action.unit.test.tspackages/project-builder-server/src/actions/definition/index.tspackages/project-builder-server/src/actions/definition/list-entities.action.unit.test.tspackages/project-builder-server/src/actions/definition/list-entity-types.action.tspackages/project-builder-server/src/actions/definition/list-entity-types.action.unit.test.tspackages/project-builder-server/src/actions/definition/list-plugins.action.tspackages/project-builder-server/src/actions/definition/list-plugins.action.unit.test.tspackages/project-builder-server/src/actions/definition/search-entities.action.tspackages/project-builder-server/src/actions/definition/search-entities.action.unit.test.tspackages/project-builder-server/src/actions/definition/stage-create-entity.action.int.test.tspackages/project-builder-server/src/actions/definition/stage-create-entity.action.tspackages/project-builder-server/src/actions/definition/stage-delete-entity.action.int.test.tspackages/project-builder-server/src/actions/definition/stage-delete-entity.action.tspackages/project-builder-server/src/actions/definition/stage-update-entity.action.int.test.tspackages/project-builder-server/src/actions/definition/stage-update-entity.action.tspackages/project-builder-server/src/actions/definition/validate-draft.tspackages/project-builder-server/src/actions/definition/validate-draft.unit.test.tspackages/project-builder-server/src/actions/registry.tspackages/tools/eslint-configs/typescript.js
💤 Files with no reviewable changes (1)
- packages/project-builder-server/src/actions/definition/definition-actions.int.test.ts
MCP server improvements: apply fixRefDeletions and applyDefinitionFixes when staging changes, add entity search action, expose auto-fix suggestions with apply-fix action, add plugin management actions (list, configure, disable), blacklist plugin entity type from generic entity operations
Summary by CodeRabbit
New Features
Bug Fixes
Changes
Tests